Skip to content

implemented sending of notifications based on segments (included/excluded)#72

Merged
LKaemmerling merged 4 commits into
laravel-notification-channels:refactor-to-traitsfrom
rylxes:add_segments
Aug 16, 2018
Merged

implemented sending of notifications based on segments (included/excluded)#72
LKaemmerling merged 4 commits into
laravel-notification-channels:refactor-to-traitsfrom
rylxes:add_segments

Conversation

@rylxes
Copy link
Copy Markdown

@rylxes rylxes commented Jul 4, 2018

ability of users to send notifications excluding a segment i.e

public function routeNotificationForOneSignal() { return ['excluded_segments' => ['excluded segments']]; }

ability of users to send notifications including a segment i.e

public function routeNotificationForOneSignal() { return ['included_segments' => ['included segments']]; }

@LKaemmerling
Copy link
Copy Markdown
Collaborator

Could you please rebase this on the refactor-to-traits branch?

@rylxes
Copy link
Copy Markdown
Author

rylxes commented Jul 5, 2018

I have done that, ... the current branch is up to date

@LKaemmerling LKaemmerling changed the base branch from master to refactor-to-traits July 5, 2018 12:42
@LKaemmerling
Copy link
Copy Markdown
Collaborator

For me this looks okay @Lloople what do you think?

@LKaemmerling LKaemmerling requested a review from Lloople August 8, 2018 07:53
Copy link
Copy Markdown
Collaborator

@Lloople Lloople left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. I personally don't like to align array values when multiline but I'm okay with it and it's totally aesthetic. Both method for include or exclude segments looks great 👍

@LKaemmerling LKaemmerling merged commit 32521dd into laravel-notification-channels:refactor-to-traits Aug 16, 2018
@rylxes rylxes deleted the add_segments branch August 18, 2018 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants